[Bug] [Segment Replication] Update store metadata recovery diff logic to ignore missing files causing exception #4185

dreamer-89 · 2022-08-10T22:03:15Z

Description

This PR changes the logic to ignore missing files which currently causes exception on replica. This issue happens when primary performs delete/update operations which generates extra files for older segment group. This PR also adds a unit test which exercises this code by generating actual segment group files.

Existing logic

for (List<StoreFileMetadata> segmentFiles : getGroupedFilesIterable()) {
 boolean consistent = true;
 for (StoreFileMetadata meta : segmentFiles) {
   StoreFileMetadata storeFileMetadata = recoveryTargetSnapshot.get(meta.name());
    if (storeFileMetadata == null) {
        consistent = false;  --> missing files (e.g. `*.liv`) makes existing segment group inconsistent
         missing.add(meta);
     } else if (storeFileMetadata.isSame(meta) == false) {
         consistent = false;
         different.add(meta);
      } else {
         identicalFiles.add(meta);
       }
     }
     if (consistent) {
          identical.addAll(identicalFiles); 
     } else {
        // make sure all files are added - this can happen if only the deletes are different
        different.addAll(identicalFiles); --> missing files causes identical files added to different list causing failures
     }

Issues Resolved

#3787

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Poojita Raj <[email protected]> Signed-off-by: Poojita Raj <[email protected]>

…n replica during copy Signed-off-by: Suraj Singh <[email protected]>

github-actions · 2022-08-10T22:28:16Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/1670/
CommitID: fe8ad1e

Poojita-Raj · 2022-08-11T06:16:44Z

 if (consistent) {
      identical.addAll(identicalFiles); 
 } else {
    // make sure all files are added - this can happen if only the deletes are different
    different.addAll(identicalFiles); --> missing files added to different list causing failures
 }

This should be -> identical files added to different list causing failure on check that files are unchanged i.e., different is empty.

Poojita-Raj · 2022-08-11T01:49:58Z

server/src/main/java/org/opensearch/index/store/Store.java

+         * <li>missing: files that exist in the source but not in the target</li>
+         * </ul>
+         */
+        public RecoveryDiff segmentReplicationDiff(MetadataSnapshot recoveryTargetSnapshot) {


Nit - rename snapshot as well for consistency - segmentReplicationTargetSnapshot

Thanks @Poojita-Raj for the review.

This method evaluates the difference in segment files b/w different store metadata snapshots which makes this naming consistent. This name also aligns with existing recoveryDiff used for evaluating snapshots diff.

Poojita-Raj · 2022-08-11T13:41:13Z

...er/src/internalClusterTest/java/org/opensearch/indices/replication/SegmentReplicationIT.java

@@ -247,6 +249,56 @@ public void testStartReplicaAfterPrimaryIndexesDocs() throws Exception {
        assertSegmentStats(REPLICA_COUNT);
    }

+    public void testDeleteOperations() throws Exception {


I ran this integ test and it still gives an off by 1 error intermittently so I think we should either hold off on it for a separate PR or fix it before merging it in.

That's a good call-out @Poojita-Raj . Yes, I am also able to repro this by running test in interations (fails 3/10). I think this is related to replica lagging behind primary and needs some deep dive. This PR is about avoid shard failures due to delete operations. I will file a new bug to track this.

./gradlew ':server:internalClusterTest' --tests "org.opensearch.indices.replication.SegmentReplicationIT.testDeleteOperations" -Dtests.seed=44620B458370EA29 -Dtests.iters=10

kartg · 2022-08-12T01:42:29Z

server/src/main/java/org/opensearch/index/store/Store.java

+         * <li>missing: files that exist in the source but not in the target</li>
+         * </ul>
+         */
+        public RecoveryDiff segmentReplicationDiff(MetadataSnapshot recoveryTargetSnapshot) {


The code in this method seems identical to recoveryDiff except for how missing files are handled. Instead of duplicating most of the code, I would suggest gating the two pieces of logic with a boolean method parameter like:

public RecoveryDiff recoveryDiff(MetadataSnapshot recoveryTargetSnapshot, boolean shouldCheckMissingFiles) {

That would also remove the need to extract the getGroupedFilesIterable method

Thanks @kartg, this is a good point! Initially, I thought of adding a flag for gating but later realized recoveryDiff is used heavily and needs lot many changes. Also, I feel keeping them separate clearly calls out for separate functionality and leaves scope for future enhancements around store's metadata comparison (for e.g. identical/missing files are not used for segment comparison and can be removed altogether) and makes it more readable.

kartg · 2022-08-12T01:45:32Z

server/src/test/java/org/opensearch/indices/replication/SegmentReplicationTargetTests.java

@@ -98,6 +122,13 @@ public void setUp() throws Exception {
        );
    }

+    private Settings getIndexSettings() {


Nitpick - any reason why this was extracted?

I think is not required. Undoing this change.

kartg · 2022-08-12T01:58:58Z

server/src/test/java/org/opensearch/indices/replication/SegmentReplicationTargetTests.java

+     */
+    List<Store.MetadataSnapshot> generateStoreMetadataSnapshot(int docCount) throws IOException {
+        List<Document> docList = new ArrayList<>();
+        for (int i = 0; i < 10; i++) {


Question - should the limit of this loop be docCount rather than 10 ?

Good catch, updating it.

kartg · 2022-08-12T02:01:09Z

server/src/test/java/org/opensearch/indices/replication/SegmentReplicationTargetTests.java

+     */
+    public void test_MissingFiles_NotCausingFailure() throws IOException {
+        int docCount = 1 + random().nextInt(10);
+        List<Store.MetadataSnapshot> storeMetadataSnapshots = generateStoreMetadataSnapshot(docCount);


Nitpick - please add comments here so that it's clear what the subsequent get(0) and get(1) fetch

Signed-off-by: Suraj Singh <[email protected]>

github-actions · 2022-08-12T05:11:21Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/1683/
CommitID: 9350ba6

codecov-commenter · 2022-08-12T05:12:50Z

Codecov Report

Merging #4185 (9350ba6) into main (a469a3c) will increase coverage by 0.08%.
The diff coverage is 82.70%.

@@             Coverage Diff              @@
##               main    #4185      +/-   ##
============================================
+ Coverage     70.59%   70.68%   +0.08%     
- Complexity    57083    57143      +60     
============================================
  Files          4603     4604       +1     
  Lines        274551   274596      +45     
  Branches      40210    40216       +6     
============================================
+ Hits         193831   194098     +267     
+ Misses        64514    64284     -230     
- Partials      16206    16214       +8

Impacted Files	Coverage Δ
...ava/org/opensearch/client/RestHighLevelClient.java	`44.32% <ø> (-0.16%)`	⬇️
...gregations/metrics/GeoBoundsAggregatorFactory.java	`88.88% <ø> (ø)`
...search/aggregations/metrics/InternalGeoBounds.java	`62.96% <ø> (ø)`
...o/search/aggregations/metrics/ParsedGeoBounds.java	`88.00% <ø> (ø)`
.../main/java/org/opensearch/search/SearchModule.java	`96.27% <ø> (-0.03%)`	⬇️
...earch/search/aggregations/AggregationBuilders.java	`46.15% <ø> (+1.15%)`	⬆️
...regations/support/AggregationInspectionHelper.java	`53.65% <ø> (+1.27%)`	⬆️
...g/opensearch/test/InternalAggregationTestCase.java	`98.21% <ø> (-0.46%)`	⬇️
...a/org/opensearch/test/OpenSearchIntegTestCase.java	`57.48% <ø> (ø)`
...regations/metrics/AbstractGeoBoundsAggregator.java	`56.52% <56.52%> (ø)`
... and 498 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

… to ignore missing files causing exception (opensearch-project#4185) * Update Store for segment replication dif Signed-off-by: Poojita Raj <[email protected]> Signed-off-by: Poojita Raj <[email protected]> * Update recoveryDiff logic to ingore missing files causing exception on replica during copy Signed-off-by: Suraj Singh <[email protected]> * Address review comments Signed-off-by: Suraj Singh <[email protected]> Signed-off-by: Poojita Raj <[email protected]> Signed-off-by: Suraj Singh <[email protected]> Co-authored-by: Poojita Raj <[email protected]>

…aining segment replication changes (#4243) * [Segment Replication] Checkpoint Replay on Replica Shard (#3658) * Adding Latest Recevied checkpoint, replay checkpoint logic along with tests Signed-off-by: Rishikesh1159 <[email protected]> * [Segment Replication] Wire up segment replication with peer recovery and add ITs. (#3743) * Add null check when computing max segment version. With segment replication enabled it is possible Lucene does not set the SegmentInfos min segment version, leaving the default value as null. Signed-off-by: Marc Handalian <[email protected]> * Update peer recovery to set the translogUUID of replicas to the UUID generated on the primary. This change updates the UUID when the translog is created to the value stored in the passed segment userdata. This is to ensure during failover scenarios that the replica can be promoted and not have a uuid mismatch with the value stored in user data. Signed-off-by: Marc Handalian <[email protected]> * Wire up Segment Replication under the feature flag. This PR wires up segment replication and adds some initial integration tests. Signed-off-by: Marc Handalian <[email protected]> * Add test to ensure replicas use primary translog uuid with segrep. Signed-off-by: Marc Handalian <[email protected]> * Update SegmentReplicationIT to assert previous commit points are valid and SegmentInfos can be built. Fix nitpicks in PR feedback. Signed-off-by: Marc Handalian <[email protected]> * Fix test with Assert.fail to include a message. Signed-off-by: Marc Handalian <[email protected]> * Disable shard idle with segment replication. (#4118) This change disables shard idle when segment replication is enabled. Primary shards will only push out new segments on refresh, we do not want to block this based on search behavior. Replicas will only refresh on an externally provided SegmentInfos, so we do not want search requests to hang waiting for a refresh. Signed-off-by: Marc Handalian <[email protected]> * Fix isAheadOf logic for ReplicationCheckpoint comparison (#4112) Signed-off-by: Suraj Singh <[email protected]> * Fix waitUntil refresh policy for segrep enabled indices. (#4137) Signed-off-by: Marc Handalian <[email protected]> * Add IndexShard#getLatestReplicationCheckpoint behind segrep enable feature flag (#4163) * Add IndexShard#getLatestReplicationCheckpoint behind segrep enable feature flag Signed-off-by: Suraj Singh <[email protected]> * Address review comment. Move tests to SegmentReplicationIndexShardTests Signed-off-by: Suraj Singh <[email protected]> * Add segrep enbaled index settings in TargetServiceTests, SourceHandlerTests Signed-off-by: Suraj Singh <[email protected]> * [Segment Replication] Fix OngoingSegmentReplications to key by allocation ID instead of DiscoveryNode. (#4182) * Fix OngoingSegmentReplications to key by allocation ID instead of DiscoveryNode. This change fixes segrep to work with multiple shards per node by keying ongoing replications on allocation ID. This also updates cancel methods to ensure state is properly cleared on shard cancel. Signed-off-by: Marc Handalian <[email protected]> * Clean up cancel methods. Signed-off-by: Marc Handalian <[email protected]> Signed-off-by: Marc Handalian <[email protected]> * [Bug] [Segment Replication] Update store metadata recovery diff logic to ignore missing files causing exception (#4185) * Update Store for segment replication dif Signed-off-by: Poojita Raj <[email protected]> Signed-off-by: Poojita Raj <[email protected]> * Update recoveryDiff logic to ingore missing files causing exception on replica during copy Signed-off-by: Suraj Singh <[email protected]> * Address review comments Signed-off-by: Suraj Singh <[email protected]> Signed-off-by: Poojita Raj <[email protected]> Signed-off-by: Suraj Singh <[email protected]> Co-authored-by: Poojita Raj <[email protected]> * [Segment Replication] Adding PrimaryMode check before publishing checkpoint and processing a received checkpoint. (#4157) * Adding PrimaryMode check before publishing checkpoint. Signed-off-by: Rishikesh1159 <[email protected]> * Applying spotless check Signed-off-by: Rishikesh1159 <[email protected]> * Moving segrep specific tests to SegmentReplicationIndexShardTests. Signed-off-by: Rishikesh1159 <[email protected]> * Adding logic and tests for rejecting checkpoints if shard is in PrimaryMode. Signed-off-by: Rishikesh1159 <[email protected]> * Applying ./gradlew :server:spotlessApply. Signed-off-by: Rishikesh1159 <[email protected]> * Applying ./gradlew :server:spotlessApply Signed-off-by: Rishikesh1159 <[email protected]> * Changing log level to warn in shouldProcessCheckpoint() of IndexShard.java class. Signed-off-by: Rishikesh1159 <[email protected]> * Removing unnecessary lazy logging in shouldProcessCheckpoint(). Signed-off-by: Rishikesh1159 <[email protected]> Signed-off-by: Rishikesh1159 <[email protected]> * [Segment Replication] Wait for documents to replicate to replica shards (#4236) * [Segment Replication] Add thread sleep to account for replica lag in delete operations test Signed-off-by: Suraj Singh <[email protected]> * Address review comments, assertBusy on doc count rather than sleep Signed-off-by: Suraj Singh <[email protected]> Signed-off-by: Suraj Singh <[email protected]> * Segment Replication - Remove unnecessary call to markAllocationIdAsInSync. (#4224) This PR Removes an unnecessary call to markAllocationIdAsInSync on the primary shard when replication events complete. Recovery will manage this initial call. Signed-off-by: Marc Handalian <[email protected]> Signed-off-by: Marc Handalian <[email protected]> * Segment Replication - Add additional unit tests for update & delete ops. (#4237) * Segment Replication - Add additional unit tests for update & delete operations. Signed-off-by: Marc Handalian <[email protected]> * Fix spotless. Signed-off-by: Marc Handalian <[email protected]> Signed-off-by: Marc Handalian <[email protected]> Signed-off-by: Rishikesh1159 <[email protected]> Signed-off-by: Marc Handalian <[email protected]> Signed-off-by: Suraj Singh <[email protected]> Signed-off-by: Poojita Raj <[email protected]> Co-authored-by: Marc Handalian <[email protected]> Co-authored-by: Suraj Singh <[email protected]> Co-authored-by: Poojita Raj <[email protected]>

Poojita-Raj and others added 2 commits August 10, 2022 11:23

Update Store for segment replication dif

b5cffe9

Signed-off-by: Poojita Raj <[email protected]> Signed-off-by: Poojita Raj <[email protected]>

Update recoveryDiff logic to ingore missing files causing exception o…

fe8ad1e

…n replica during copy Signed-off-by: Suraj Singh <[email protected]>

dreamer-89 requested review from a team and reta as code owners August 10, 2022 22:03

Poojita-Raj reviewed Aug 11, 2022

View reviewed changes

dreamer-89 mentioned this pull request Aug 11, 2022

[Segment Replication] [BUG] Document update action fails segment replication #3787

Closed

kartg reviewed Aug 12, 2022

View reviewed changes

Address review comments

9350ba6

Signed-off-by: Suraj Singh <[email protected]>

Poojita-Raj approved these changes Aug 12, 2022

View reviewed changes

dreamer-89 merged commit b9a64cb into opensearch-project:main Aug 12, 2022

This was referenced Aug 15, 2022

[Segment Replication] [BUG] Deleted docs remain searchable on replica after refresh #4216

Closed

[Segment Replication] Add update doc integ test for Segment replication #4234

Merged

Rishikesh1159 mentioned this pull request Aug 17, 2022

[Backport 2.x] [Segment Replication] Backport all PR's containing remaining segment replication changes #4243

Merged

kartg mentioned this pull request Aug 30, 2022

Adding @dreamer-89 to Opensearch maintainers. #4342

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Bug] [Segment Replication] Update store metadata recovery diff logic to ignore missing files causing exception #4185

[Bug] [Segment Replication] Update store metadata recovery diff logic to ignore missing files causing exception #4185

dreamer-89 commented Aug 10, 2022 •

edited

Loading

github-actions bot commented Aug 10, 2022

Poojita-Raj commented Aug 11, 2022

Poojita-Raj Aug 11, 2022

dreamer-89 Aug 11, 2022

Poojita-Raj Aug 11, 2022

dreamer-89 Aug 11, 2022 •

edited

Loading

kartg Aug 12, 2022

dreamer-89 Aug 12, 2022

kartg Aug 12, 2022

dreamer-89 Aug 12, 2022

kartg Aug 12, 2022

dreamer-89 Aug 12, 2022

kartg Aug 12, 2022

dreamer-89 Aug 12, 2022

github-actions bot commented Aug 12, 2022

codecov-commenter commented Aug 12, 2022

[Bug] [Segment Replication] Update store metadata recovery diff logic to ignore missing files causing exception #4185

[Bug] [Segment Replication] Update store metadata recovery diff logic to ignore missing files causing exception #4185

Conversation

dreamer-89 commented Aug 10, 2022 • edited Loading

Description

Related

Issues Resolved

github-actions bot commented Aug 10, 2022

Gradle Check (Jenkins) Run Completed with:

Poojita-Raj commented Aug 11, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dreamer-89 Aug 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Aug 12, 2022

Gradle Check (Jenkins) Run Completed with:

codecov-commenter commented Aug 12, 2022

Codecov Report

dreamer-89 commented Aug 10, 2022 •

edited

Loading

dreamer-89 Aug 11, 2022 •

edited

Loading